Commonmarker 1.0 support #1540

ParadoxV5 · 2024-03-23T02:01:29Z

Description

Add Commonmarker (all lower-case ms) provider for commonmarker ~> 1.0.0
- Fixes Commonmarker 1.0 support #1528
Remove tests for Markdown header IDs or the lack of, considering they may improve with newer provider versions and that YARD shouldn’t constraint their growth
Also change “Missing markup renderer” from skip to raise to catch this sort of issues in the future

Completed Tasks

I have read the Contributing Guide.
The pull request is complete (implemented / written).
Git commits have been cleaned up (squash WIP / revert commits).
I wrote tests and ran bundle exec rake locally (if code is attached to PR).

Fixes lsegal#1528 Co-Authored-By: Andrew Haines <[email protected]>

Preference for the provider’s highlighting is outside of this PR’s scope.

ParadoxV5 · 2024-03-23T02:02:58Z

lib/yard/templates/helpers/html_helper.rb

@@ -89,6 +89,9 @@ def html_markup_markdown(text)
                             :tables,
                             :with_toc_data,
                             :no_intraemphasis).to_html
+        when 'Commonmarker'
+          # GFM configs are on by default; use YARD for syntax highlighting
+          Commonmarker.to_html(text, plugins: {syntax_highlighter: nil})


Suggested change

Commonmarker.to_html(text, plugins: {syntax_highlighter: nil})

Commonmarker.to_html(text)

However, preference for the provider’s highlighting is outside of this PR’s scope.

Testing providers’ implementation is outside of the scope of YARD. I let the backslash line break difference live since that’s probably provider-specific.

lsegal

There's an issue in supporting both CommonMarker and Commonmarker in this PR. At first glance it seems like YARD is no longer able to support the pre 1.0 commonmarker lib due to a change in the markup_helper lookup table. If we can address that, I think this mostly looks good.

lsegal · 2024-08-26T07:07:46Z

spec/templates/markup_processor_integrations/integration_spec_helper.rb

@@ -26,7 +26,7 @@

  before(:each) do
    if html_renderer.markup_class(markup).nil?
-      skip "Missing markup renderer #{markup}"
+      raise "Missing markup renderer #{markup}"


This should stay as skip for systems that do not have the dependencies installed (testing outside of bundle exec).

I am unable to run rake/rspec locally without a prior bundle install.

Could not find redcarpet-3.6.0 in locally installed gems Run `bundle install` to install missing gems. While loading spec_helper an `exit` / `raise SystemExit` occurred, RSpec will now quit. Failure/Error: require 'bundler/setup' SystemExit: exit # ./spec/spec_helper.rb:10:in `<top (required)>' # ------------------ # --- Caused by: --- # Bundler::GemNotFound: # Could not find redcarpet-3.6.0 in locally installed gems # ./spec/spec_helper.rb:10:in `<top (required)>'

yard/spec/spec_helper.rb

Lines 9 to 13 in ad2c1c4

begin

require 'bundler/setup'

rescue LoadError

nil # noop

end

lsegal · 2024-08-26T07:13:34Z

lib/yard/templates/helpers/markup_helper.rb

@@ -30,6 +30,7 @@ def clear_markup_cache
          {:lib => :maruku, :const => 'Maruku'},
          {:lib => :'rpeg-markdown', :const => 'PEGMarkdown'},
          {:lib => :rdoc, :const => 'YARD::Templates::Helpers::Markup::RDocMarkdown'},
+          {:lib => :commonmarker, :const => 'Commonmarker'},


This is problematic, as the lib value is a unique key passed via -m. This will effectively mean that the CommonMarker (pre 1.0?) class could never be activated. We should definitely maintain support for the old format, which would require this commonmarker key be changed-- it's not idela, but maybe :commonmarker1?

CommonMarker seems still accessible.

yard/lib/yard/templates/helpers/markup_helper.rb

Lines 91 to 110 in ad2c1c4

providers = MARKUP_PROVIDERS[type.to_sym]

return true if providers && providers.empty?

if providers && options.markup_provider

providers = providers.select {|p| p[:lib] == options.markup_provider }

end

if providers.nil? || providers.empty?

log.error "Invalid markup type '#{type}' or markup provider " \

"(#{options.markup_provider}) is not registered."

return false

end

# Search for provider, return the library class name as const if found

providers.each do |provider|

begin require provider[:lib].to_s; rescue LoadError; next end if provider[:lib]

begin klass = eval("::" + provider[:const]); rescue NameError; next end # rubocop:disable Lint/Eval

MarkupHelper.markup_cache[type][:provider] = provider[:lib] # Cache the provider

MarkupHelper.markup_cache[type][:class] = klass

return true

end

Even if it’s not, v1.0 CommonMaker should have priority over pre-1.0 Commonmarker.
Separating a commonmarker1 key keeps commonmarker out of date.
YARD does not have a system that checks provider versions and has long¹ exposed users to Commonmarker 1.0’s backward incompatibility. We should improve this compatibility rather than make users work an extra step.

Footnotes

Commonmarker 1.0 released last Christmas Eve! ↩

Even if it’s not, v1.0 CommonMaker should have priority over pre-1.0 Commonmarker.

Unfortunately this would be a breaking change and would need a major version bump-- we're very unlikely to major version bump YARD just for a single (optional) markup library.

YARD is currently still in 0.x versions, so I’m not sure what you meant by ‘major’.

That said, together with #1540 (comment), I see your concern.
YARD currently has designs tailored for CommonMarker v0.x but it’s not documented anywhere.
We can only treat Commonmarker v1.x as a separate provider.

YARD is currently still in 0.x versions, so I’m not sure what you meant by ‘major’.

YARD's 0.x releases are a major version. Believe it or not, there was a time before "SemVer", and YARD predates the existence of the now-popular semantic versioning specification, along with its arbitrary distinction that only 1.0+ are considered API stable. YARD's 0.x releases are considered a stable API. "0" is the current major version for YARD, we simply use 0-based indexing.

YARD does not have a system that checks provider versions

We might wanna add one. But for now, …

YARD currently has designs tailored for CommonMarker v0.x but it’s not documented anywhere.

… I’ll add a comment in the code since it’s now linked in the README.

Prerequisite: documentation about markdown providers #1544

At first glance it seems like YARD is no longer able to support the pre 1.0 commonmarker lib due to a change in the markup_helper lookup table.

Actually, we might not be able to support both Commonmarker and CommomMarker at all.
Commonmarker 1.x shadows CommonMarker 0.x. We’d have to write individual Bundler.requires in our specs.

lsegal · 2024-08-26T07:14:56Z

spec/templates/markup_processor_integrations/markdown_spec.rb

    let(:markup) { :markdown }
    let(:markup_provider) { :commonmarker }

    include_examples 'shared examples for markdown processors'

-    it 'generates level 2 header without id' do


This should remain but should be scoped to CommonMarker

Why? Does it have an impact?

Testing providers’ implementation is outside of the scope of YARD.

Remove tests for Markdown header IDs or the lack of, considering they may improve with newer provider versions and that YARD shouldn’t constraint their growth

Testing providers’ implementation is outside of the scope of YARD.

Not when we're explicitly testing system integration. The fact that this behavior changes is exactly why we have tests. An id-less h2 heading actually has implications for downstream YARD code, since we have special code paths for heading id attributes.

Actually, expanding this comment outward, it seems like this change removes all integration tests for CommonMarker, which means we're now only testing Commonmarker 1.0. This is also insufficient. We should keep the old tests for CommonMarker and add the new ones for this 1.0 version, not replace.

tl;dr we need to test both CommonMarker / Commonmarker implementations. Not just one. You can remove the h2 test for Commonmarker 1.0 if you want, but both libraries should be integration tested.

Remove tests for Markdown header IDs or the lack of, considering they may improve with newer provider versions and that YARD shouldn’t constraint their growth

YARD relies on this IDs though, that's the issue, and that's why we test.

ParadoxV5 and others added 3 commits March 22, 2024 17:22

Surface missing/incompatible markups rather than skip

2c0bac2

Support Commonmarker (lowercase m) from v1.x

3472342

Fixes lsegal#1528 Co-Authored-By: Andrew Haines <[email protected]>

Disable Commonmarker syntax highlighting for compatibility

0e274a0

Preference for the provider’s highlighting is outside of this PR’s scope.

ParadoxV5 commented Mar 23, 2024

View reviewed changes

Remove tests for Markdown header IDs or the lack of

9e869c9

Testing providers’ implementation is outside of the scope of YARD. I let the backslash line break difference live since that’s probably provider-specific.

ParadoxV5 marked this pull request as ready for review March 23, 2024 18:34

ParadoxV5 added a commit to ParadoxV5/template-ruby-gem that referenced this pull request Mar 23, 2024

Use my tag for lsegal/yard#1540

a89e3eb

lsegal requested changes Aug 26, 2024

View reviewed changes

ParadoxV5 marked this pull request as draft August 26, 2024 22:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Commonmarker 1.0 support #1540

Commonmarker 1.0 support #1540

ParadoxV5 commented Mar 23, 2024 •

edited

Loading

ParadoxV5 Mar 23, 2024

lsegal left a comment

lsegal Aug 26, 2024

ParadoxV5 Aug 26, 2024 •

edited

Loading

lsegal Aug 26, 2024

ParadoxV5 Aug 26, 2024

lsegal Aug 26, 2024

ParadoxV5 Aug 26, 2024

lsegal Aug 26, 2024

ParadoxV5 Aug 26, 2024

ParadoxV5 Aug 26, 2024

lsegal Aug 26, 2024

ParadoxV5 Aug 26, 2024

lsegal Aug 26, 2024

lsegal Aug 26, 2024

	Commonmarker.to_html(text, plugins: {syntax_highlighter: nil})
	Commonmarker.to_html(text)

	providers = MARKUP_PROVIDERS[type.to_sym]
	return true if providers && providers.empty?
	if providers && options.markup_provider
	providers = providers.select {\|p\| p[:lib] == options.markup_provider }
	end

	if providers.nil? \|\| providers.empty?
	log.error "Invalid markup type '#{type}' or markup provider " \
	"(#{options.markup_provider}) is not registered."
	return false
	end

	# Search for provider, return the library class name as const if found
	providers.each do \|provider\|
	begin require provider[:lib].to_s; rescue LoadError; next end if provider[:lib]
	begin klass = eval("::" + provider[:const]); rescue NameError; next end # rubocop:disable Lint/Eval
	MarkupHelper.markup_cache[type][:provider] = provider[:lib] # Cache the provider
	MarkupHelper.markup_cache[type][:class] = klass
	return true
	end

Commonmarker 1.0 support #1540

Are you sure you want to change the base?

Commonmarker 1.0 support #1540

Conversation

ParadoxV5 commented Mar 23, 2024 • edited Loading

Description

Completed Tasks

Choose a reason for hiding this comment

lsegal left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Footnotes

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ParadoxV5 commented Mar 23, 2024 •

edited

Loading

ParadoxV5 Aug 26, 2024 •

edited

Loading